Repository navigation
Compute dig time from block tags when the registry has them - #127
Pix3lPirat3 wants to merge 3 commits into
Conversation
When registry.tags carries block tags (minecraft-data 1.17+), take the breaking speed from mineable/* membership and the tool's tier speed in materials.json, with the vanilla sword and shears rules (cobweb 15, sword_efficient 1.5, sword_instantly_mines instant; leaves 15, wool 5, vine/glow_lichen 2), instead of the single material table. A block can be in several mineable tags, and the harvest gate stays on harvestTools. Without tags the existing material path is unchanged.
|
The tag path gives the expected times for the hoe/axe cases I raised on generator #86. I also found an older-version regression here: with minecraft-data #1308's actual tags, a diamond sword on oak leaves takes 300 ms versus 200 ms with the base consumer on 1.17.1, 1.18.2 and 1.19.4. Those versions have Source: disassembly of the official 1.17.1 server jar, using its official mappings. |
Before 1.20 SwordItem.getDestroySpeed gives 1.5 for the PLANT, REPLACEABLE_PLANT and VEGETABLE materials and the leaves tag; there is no sword_efficient tag to read, so the tag path returned 1 and a diamond sword on leaves took 300 ms instead of 200 ms on 1.17 to 1.19. When the registry has no sword_efficient tag, take the sword speed from the material table of those versions, which encodes exactly those rules. Bamboo and bamboo saplings break instantly with a sword through BambooStalkBlock/BambooSaplingBlock.getDestroyProgress until 1.21.4, the sword_instantly_mines tag only exists from 1.21.5; special-case the two blocks when the tag is absent. Checked every diggable block against every sword on 1.17.1, 1.18.2 and 1.19.4 with the real tags: the tag path now equals the material path everywhere except bamboo, where it is instant like the game.
|
Good catch, thanks. Pushed a fix: when the registry has no |
SwordItem.getDestroySpeed is 1.5 for the leaves tag and the plant, vegetable and vine materials on 1.17-1.19. Reading it from the material table's composite sword speeds broke once tags were regenerated (leaves dropped from 1.5 to 1); derive it from the leaves tag and material names instead. Covers vine in the older-version test.
|
Fixed in 528850d. The pre-1.20 sword rule is now derived directly from the leaves tag and the plant/gourd/vine materials (SwordItem.getDestroySpeed's ToolMaterial constants), instead of the material table's composite sword speeds - which drop from 1.5 to 1 once tags are regenerated, exactly the leaves case you caught. Added a vine case to the 1.17-1.19 test alongside the existing leaves/melon coverage; full suite (8190) stays green. |
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
Looks ready to merge from this review. The revised historical sword branch fixes the reported older-version case: using the actual candidate tags from minecraft-data#1308, leaves/vine/melon and instant bamboo checks pass on 1.17.1, 1.18.2 and 1.19.4, alongside representative newer versions. I also checked the ore tier-speed path and unchanged harvest penalty. Full suite: 8,190 passing at head versus 8,180 at base; changed-file lint and current CI pass.
There is a release prerequisite for activating the new path: my installed minecraft-data 3.116.0 does not expose tags. The model checks explicitly supplied #1308's candidate tags. With existing released data the preserved material fallback remains active; publication of the data and node-minecraft-data#457 is needed for users to receive the new tag behavior automatically. No new live-server test was run.
Skills used: prismarine-protocol-data-review checked actual selected data and the wrapper/release boundary; prismarine-architecture-review checked that block breaking remains in the block model with a compatible fallback; prismarine-review verified the historical feedback was repaired before reassessing.
| const item = registry.items[heldItemType] | ||
| if (!item) return 1 | ||
| const name = block.name | ||
| if (item.name === 'shears') { |
There was a problem hiding this comment.
This kind of hard coding is not future proof how can we do better?
There was a problem hiding this comment.
Agreed these constants are not future proof. The good news is the speeds are not actually unique to this code: they already exist in registry.materials, keyed by the block's (possibly composite) material and the item id. For example on released 1.17 through 1.21 data:
materials["leaves;mineable/hoe"]has{ <shears>: 15, <sword>: 1.5, <hoe>: ... }materials.woolhas{ <shears>: 5 },materials.vine_or_glow_lichenhas{ <shears>: 2, <sword>: 1.5 },materials.cowebhas{ <shears>: 15, <sword>: 15 }
So instead of the shears/sword constants we can read the speed straight from the material table: registry.materials[block.material]?.[heldItemType] ?? 1. I prototyped that and compared it against the current hard-coded version across every block, 11 tools (hand, shears, each sword/pickaxe/axe/shovel/hoe tier) and 9 versions (1.16.5 to 1.21.11): 98,835 (block, tool, version) cells, 0 differences. The only speed not in the material table is a sword mining bamboo instantly, which is a getDestroyProgress override rather than a tool speed; that stays as the sword_instantly_mines tag (1.21.5+) or the bamboo name before it.
The one caveat is the one that led me to hard-code them originally: the generator can drop these composite speeds when the data is regenerated (the leaves-sword-1.5-to-1 regression), which is being fixed separately in minecraft-data-generator#88. Released data carries them correctly, so a consumer reading registry.materials is safe today.
The tradeoff is that this sources the speed from registry.materials rather than the tag set, so the dispatch becomes material-driven and the current tag-fixture tests would move to material-based assertions. I did not want to pivot the PR's approach without your call. Want me to take the material-table direction (I will convert the tests to match), or keep the tag dispatch and just document these as the vanilla ToolMaterial/ShearsItem constants?
When registry.tags carries block tags (minecraft-data 1.17+), take the breaking speed from mineable/* membership and the tool's tier speed in materials.json, with the vanilla sword and shears rules (cobweb 15, sword_efficient 1.5, sword_instantly_mines instant; leaves 15, wool 5, vine/glow_lichen 2), instead of the single material table. A block can be in several mineable tags, and the harvest gate stays on harvestTools. Without tags the existing material path is unchanged.
Since 1.17 the game decides which tools mine a block by block tags (
mineable/*,needs_*_tool, laterincorrect_for_*_tool), whilematerials.jsoncan only name one material per block. That is how tier-gated blocks ended up withincorrect_for_wooden_toolas their material anddigTime()fell back to hand speed (#69, PrismarineJS/minecraft-data#987, PrismarineJS/mineflayer#3921, PrismarineJS/mineflayer#4131), and why the sword/shears cases keep drifting inmaterials.json(shears on leaves and wool are 1.0 there from 1.20.5; vanilla is 15 and 5).With PrismarineJS/minecraft-data#1308 (tags for every version from 1.13) and PrismarineJS/node-minecraft-data#457 (
mcData.tags),registry.tags['minecraft:block']is available. When it hasmineable/pickaxe,digTime()now takes the breaking speed from tags: the tool's tier speed frommaterials['mineable/<type>']if the block is in that tag (a block may be in several), and the vanillaToolMaterial/ShearsItemrules for swords and shears, which live in code rather than in any data file.canHarvest()and the 30/100 divisor are unchanged, as are efficiency, haste, fatigue, water and airborne handling. Without tags - pre-1.17, bedrock, older minecraft-data - the existing material path runs exactly as before.Verification: every diggable block x every tool tier/type, swords, shears and hand, on 1.17.1, 1.18.2, 1.19.4, 1.20.4, 1.20.5, 1.21.4, 1.21.11 and 26.1 (21k–36k combinations each), matches the vanilla formula using tag membership, with
blocks.jsonleft as shipped. Live on vanilla 26.1 with mineflayer 4.39.0, the tag path's number drove each dig and the server accepted every one: iron ore with a diamond pickaxe 607 ms (material path: 4550), obsidian 9404 ms (75000), leaves with shears instant (300), wool with shears 251 ms (1200).